Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Respect end-tracing script instead of deleting one variable #938

Merged
merged 2 commits into from
Feb 23, 2022

Conversation

edoardopirovano
Copy link
Contributor

This might fix an issue we've observed with the CLR tracing continuing to trace after we've supposedly stopped tracing. It also just seems the more correct thing to do anyway. The variable that was previously deleted (ODASA_TRACER_CONFIGURATION) is always unset by end-tracing, so we should only be doing more to try and halt tracing.

Merge / deployment checklist

  • Confirm this change is backwards compatible with existing workflows.
  • Confirm the readme has been updated if necessary.
  • Confirm the changelog has been updated if necessary.

@edoardopirovano edoardopirovano requested a review from a team as a code owner February 17, 2022 15:47
@edoardopirovano edoardopirovano force-pushed the respect-end-tracing branch 2 times, most recently from 3f68d85 to 84ab7c2 Compare February 17, 2022 15:57
Copy link
Contributor

@criemen criemen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM from my side, but a JS person should review as well.
I checked that for C#, end-tracing.json indeed contains (besides others)

"COR_ENABLE_PROFILING":null,"COR_PROFILER":null,"COR_PROFILER_PATH_64":null,"CORECLR_ENABLE_PROFILING":null,"CORECLR_PROFILER":null

Comment on lines 29 to 53
const endTracingEnvVariables: Map<string, string | null> = JSON.parse(
fs.readFileSync(
path.resolve(
config.dbLocation,
"temp/tracingEnvironment/end-tracing.json"
),
"utf8"
)
);
for (const [key, value] of Object.entries(endTracingEnvVariables)) {
if (value !== null) {
process.env[key] = value;
} else {
delete process.env[key];
}
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you wrap this in a try-catch and rethrow with a good error message if the file doesn't exist or is malformed?

Copy link
Contributor

@aeisenberg aeisenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small suggestion, but I think it is important to help avoid potential problems with users' workflows.

@edoardopirovano
Copy link
Contributor Author

Thanks @aeisenberg, I've added some more error handling!

I note that it would be nice to have this for the start of tracing as well. The natural thing to do would be to have a utility function that can parse either file and contains all the error handling. Unfortunately, it's not quite that easy because the types of the return value are different as the end tracing file can contain null values to unset environment variables whereas the start tracing one cannot, so we'd have to change the types of quite a few things all around the code that use the start tracing file.

I propose leaving that extension for a future PR since that seems to be growing the scope of this PR too much.

@edoardopirovano edoardopirovano merged commit d7ad71d into main Feb 23, 2022
@edoardopirovano edoardopirovano deleted the respect-end-tracing branch February 23, 2022 17:08
@github-actions github-actions bot mentioned this pull request Feb 23, 2022
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants